Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Argument for Filtering Token Types #34

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add Argument for Filtering Token Types #34

wants to merge 5 commits into from

Conversation

apackin
Copy link
Contributor

@apackin apackin commented Jun 17, 2024 •
edited
Loading

Addresses #32

Pull Request: Add Argument for Filtering Token Types

Overview

This pull request introduces a new argument, filteredTokenTypes, to exclude specified token types from parsed results:

  • New Argument: filteredTokenTypes
  • Functionality: Omits specified token types from processing
  • Usage: --filtered-token-types=core,source

Files Modified

  1. bin/figma2flutter.dart

    • Main Function:
      • Added filteredTokenTypes option
      • Parsed argument into a list of token types
      • Passed filteredTypes to _processTokens
    • Token Processing:
      • Implemented filtering logic in _processTokens

  2. lib/config/options.dart

    • Option Definitions:
      • Added filteredTokenTypes constants and option for configuration

  3. lib/processor.dart

    • Processor Class:
      • Updated process method to accept filteredTypes
      • Added logic to remove tokens based on filteredTypes list

Example Usage

To run the tool and exclude tokens of types core and source:

dart bin/figma2flutter.dart --filtered-token-types=core,source

This will filter out tokens with paths starting with core or source.

Conclusion

These updates provide more control over the token processing, enabling users to fine-tune their output. Please review the changes and provide your feedback.

Notes:

I tried skipping reading the "Core" tokens altogether, but other tokens rely on their processing to assign the correct values.

@apackin apackin mentioned this pull request Jun 17, 2024
Open
@frederikstonge
Copy link
Collaborator

It's a bit too specific imo. Adding a filter options to filter an array of token types would be a better solution.

You have removed a lot of comments, I don't think it was necessary.

Please add unit tests.

Please update documentation.

@apackin apackin changed the title add flag for skipping "core" tokens add an argument for filteredTokenTypes to be omitted from parsed results Jun 17, 2024
@apackin apackin changed the title add an argument for filteredTokenTypes to be omitted from parsed results Add Argument for Filtering Token Types Jun 17, 2024
@apackin
Copy link
Contributor Author

apackin commented Jun 17, 2024 •
edited
Loading

Thank you for the quick comments. I made the recommended code changes. Let me know if that looks good and I'll update with tests and docs later

@frederikstonge
Copy link
Collaborator

Thank you for the quick comments. I made the recommended code changes. Let me know if that looks good and I'll update with tests and docs later

Looks good to me :)

@freemansoft
Copy link
Collaborator

Should this be filteredTokenPaths or filteredTokenSets in place of filteredTokenTypes?

This filtering does what? It filters out all the tokens that were part of a token set while letting them be used as a base value for tokens that are retained?

@apackin
Copy link
Contributor Author

apackin commented Jun 18, 2024 •
edited
Loading

It filters out all the tokens that were part of a token set

Yes, that is the intended usage. Technically there isn't enforcement regarding whether the filtered out items were used as a base value for other tokens, so it could be used to omit sets for other reasons if those exist. Happy to rename to filteredTokenSets

@apackin
Copy link
Contributor Author

apackin commented Jun 21, 2024

I renamed the argument and added tests

freemansoft
freemansoft reviewed Jun 26, 2024
View reviewed changes
lib/processor.dart Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freemansoft freemansoft freemansoft left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@apackin @frederikstonge @freemansoft